Tests and bug fixes for XmlRpcServer #1243
Conversation
| @@ -3,6 +3,23 @@ if(TARGET xmlrpcvalue_base64) | |||
| target_link_libraries(xmlrpcvalue_base64 xmlrpcpp) | |||
| endif() | |||
|
|
|||
| # Some of the tests that follow use boost threads. | |||
There was a problem hiding this comment.
Please add <test_depend>boost</test_depend>, since inclusion of this file is guarded by CATKIN_ENABLE_TESTING.
| ASSERT_TRUE(c.executeNonBlock("Hello", noArgs)); | ||
|
|
||
| bool done = false; | ||
| for (int i = 0; i < 30; i++) { |
There was a problem hiding this comment.
Since this is a new file, ROS brace style, please (here and elsewhere). If you like, run astyle on it:
| @@ -32,14 +56,14 @@ XmlRpcServer::~XmlRpcServer() | |||
|
|
|||
|
|
|||
| // Add a command to the RPC server | |||
| void | |||
| void | |||
There was a problem hiding this comment.
Please revert these whitespace changes (a follow-on commit which re-adds them is acceptable, as it will get squashed down anyway).
| methods[2] = "Sum"; | ||
| methods[3] = "system.listMethods"; | ||
| methods[4] = "system.methodHelp"; | ||
| methods[5] = "system.multicall"; |
There was a problem hiding this comment.
I'm partial to boost::assign::list_of in these instances, but what's here is fine.
|
Looks great. Will wait for @dirk-thomas to weigh in, but this is a terrific set of validations, thank you! |
Add integration tests for the XmlRpcServer interacting with an XmlRpcClient in the same process, particularly around the behavior when the server process runs out of available file handles. Update the XmlRpcServer with two different mitigations for file handle exhaustion (Fixes ros#914, replaces ros#960 and ros#977): * Measure the number of free file handles and reject incoming connections if the pool of free file descriptors is too small. This actively rejects incoming clients instead of waiting for complete file handle exhaustion, which would leave clients in a pending state until a file descriptor becomes free. * If accept fails due to complete file handle exhaustion, temporarily stop calling accept on this socket. This prevents a busy-loop where poll() believes the listening socket is readable, but accept() fails to allocate a file descriptor and leaves the socket in a readable state.
trainman419
force-pushed
the
trainman419/fix_xmlrpcpp_bugs_6
branch
from
69f3d29 to
e0cf7a0
Compare
|
The patch itself looks fine to me. It does break ABI though so I am (as always) hesitant to merge this into an already released distro. I guess the argument will be "it doesn't matter for Lunar to break ABI and we want the patch now instead of for Melodic"? |
|
I don't really have a stake in which version this merges into. I will say that it fixes a fairly substantial bug for our use case, so I think it there is a strong argument for including it in Lunar. Since this isn't an API that is used by most clients, and it will be released in sync with roscpp, I think the impact of the ABI breakage will be low. |
|
Thank you for the patch. |
|
Looks like this might have broken roscore on macos: #1357 |
Add integration tests for the XmlRpcServer interacting with an XmlRpcClient in the same process, particularly around the behavior when the server process runs out of available file handles.
Update the XmlRpcServer with two different mitigations for file handle exhaustion (Fixes #914, replaces #960 and #977):
handle exhaustion, which would leave clients in a pending state until a file descriptor becomes free.
allocate a file descriptor and leaves the socket in a readable state.